Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: support Terra Classic v1.1.0 #348

Merged
merged 1 commit into from
Mar 28, 2023
Merged

feat: support Terra Classic v1.1.0 #348

merged 1 commit into from
Mar 28, 2023
+1,182 −6,365

Conversation

inon-man
Copy link
Contributor

@inon-man inon-man commented Feb 23, 2023 •
edited
Loading

Description

This PR adds burn tax split and tax exemption feature to the treasury module to support Terra Classic v1.1.0. This update will solve error on the Station when the Burn Tax Exemption proposal is added to the network.

Changes

  • Add burn_tax_split to treasury params
  • Add AddBurnTaxExemptionAddressProposal and RemoveBurnTaxExemptionAddressProposal
  • Replace legacy.proto to terra.proto where terra.proto should be used.
  • Update protobuf package for Terra Classic, @terra-money/terra.proto@0.1.7, to @classic-terra/terra.proto@1.1.0
  • Update version of GitHub workflow in the deprecate state

@inon-man inon-man mentioned this pull request Feb 23, 2023
Closed
@inon-man inon-man force-pushed the feat-burn-tax branch 2 times, most recently from 637bf5d to 42c51e2 Compare February 24, 2023 10:01
@inon-man
* Replace legacy.proto to terra.proto where terra.proto should be used.
59f02f8 
* Update protobuf package for Terra Classic, @terra-money/terra.proto@0.1.7, to @classic-terra/terra.proto@1.1.0
* Add burn_tax_split to treasury params
* Add AddBurnTaxExemptionAddressProposal and RemoveBurnTaxExemptionAddressProposal
* Update version of GitHub workflow in the deprecate state
@ZaradarBH
Copy link

@Jared-TFL you seems to be stating on Twitter that there is something wrong with our PR @ https://twitter.com/ZaradarBH/status/1631696396746145792 . Could anyone in TFL please give us some actual feedback on what the issues are so we can fix them and get the PR approved? Thx.

@alecande11
Copy link
Contributor

Hey, what's the reason to switch to an external npm package for protobuf?

@inon-man
Copy link
Contributor Author

inon-man commented Mar 4, 2023 •
edited
Loading

Hey, what's the reason to switch to an external npm package for protobuf?

terra.js is using two version of same npm package '@terra-money/terra.proto'

For Terra Classic: https://github.com/terra-money/terra.js/blob/main/package.json#L86
For Terra 2.0: https://github.com/terra-money/terra.js/blob/main/package.json#L87

However, there is a protobuf update for Terra Classic v1.1.0 (PR: classic-terra/terra.proto#1)
I created a separate repository, https://github.com/classic-terra/terra.proto, and posted an npm package as well, assuming that terra-money/terra.proto will no longer retain the terra.proto version for classic.

I carefully reviewed all the codes that use legacy.proto (same terra.proto but renamed by npm package feature in package.json) and modified the codes that use legacy.proto unnecessarily to use terra.proto.

If you intend to continue maintaining terra-money/terra.proto updates for Classic, I can create a separate PR to terra-money/terra.proto. In this case:

  1. This PR will involve quite a lot of code changes as you can see in feat: burn tax split and tax exemption classic-terra/terra.proto#1
    Terra.proto takes terra-money/core as a submodule and creates .js, .py, and .java files from the .proto files there. However, the source code of terra-money/core has a very different structure from the core of Terra Classic, making it inappropriate to create files through submodules. Therefore, I copied all the proto files into terra.proto project instead of using the submodule method.
  2. num publish is required in @terra-money/terra.proto@0.1.x version.

@ZaradarBH ZaradarBH mentioned this pull request Mar 6, 2023
Closed
6 tasks
@emidev98 emidev98 merged commit 1d759c1 into terra-money:main Mar 28, 2023
@ZaradarBH ZaradarBH mentioned this pull request Mar 28, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@apepayer apepayer apepayer approved these changes

@emidev98 emidev98 emidev98 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@inon-man @ZaradarBH @alecande11 @emidev98 @apepayer